Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow for Abstract Effect Type in Schema Gen #951

Merged
merged 3 commits into from
Jul 7, 2021
Merged

Allow for Abstract Effect Type in Schema Gen #951

merged 3 commits into from
Jul 7, 2021
+126 −24

Conversation

LaurenceWarne
Copy link
Contributor

Hi! This PR allows the effect type of generated the generated Queries and Mutations classes to be abstract, ie:

case class Mutations[F[_]](
    foo: FooArgs => F[Foo],
    bar: BarArgs => F[Bar]
)

To do this, you can use the --abstractEffectType argument (hopefully I've added doc in the right place for this).

Currently the effect parameter will be used as the abstract type value so to get the above example you would have to --abstractEffectType true --effect F, though I'm unsure if this is best since effect defaults to the concrete type zio.UIO (so you would get Mutations[UIO[_]] without specifying F 😨 ). I was thinking maybe ignore the value of effect if --abstractEffectType is set and always use F, what do you think?

Let me know what you think... or if this is just a terrible idea 🙂

LaurenceWarne
LaurenceWarne commented Jul 6, 2021
View reviewed changes
tools/src/main/scala/caliban/tools/SchemaWriter.scala
def writeSubscriptionField(field: FieldDefinition, od: ObjectTypeDefinition)(implicit
scalarMappings: ScalarMappings
): String =
): String =
Copy link
Contributor Author

@LaurenceWarne LaurenceWarne Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sbt fmt chose to do this :D

@LaurenceWarne
Copy link
Contributor Author

Unsure why CI is failing due to a time out, sbt ++2.12.13! codegenSbt/scripted passes locally for me?

@ghostdogpr
Copy link
Owner

For some reason the 2.12 compilation of core times out (10 min) since a few days ago, not sure why. It takes a couple minutes max locally, weird. I'll review your PR tomorrow and restart the CI job if needed.

@ghostdogpr
Copy link
Owner

I merged #952 which seems to improve it, can you rebase so that you have it too?

@LaurenceWarne
Allow for abstracted effect type in schema gen
ed2cfa5 
Allow for the use of an abstracted effect type in schema generation
and add tests for usage.
@LaurenceWarne LaurenceWarne force-pushed the abstract-effect-type branch from 03f78a0 to f9f85d5 Compare July 6, 2021 12:33
@LaurenceWarne
Copy link
Contributor Author

Done 👍

@ghostdogpr
Copy link
Owner

Looks good! How about using F as a default when --abstractEffectType is true and --effect is not given? And keep UIO when it's false or missing?

@LaurenceWarne
Add abstractEffectType as a cmdline option
94514c3 
Add --abstractEffectType as a command line option and add tests to check
it's parsed correctly.
@LaurenceWarne
Add documentation around abstractEffectType option
5a0e2aa 
Add to documentation in schema.md around usage of the
--abstractEffectType option.
@LaurenceWarne LaurenceWarne force-pushed the abstract-effect-type branch from f9f85d5 to 5a0e2aa Compare July 7, 2021 10:31
@LaurenceWarne
Copy link
Contributor Author

Looks good! How about using F as a default when --abstractEffectType is true and --effect is not given? And keep UIO when it's false or missing?

Yeah that makes sense, I've made a few changes for this 👍

Codegen.scala does not appear to have any unit tests, so I've not added a test for if abstractEffectType is true set effect to F else set to UIO, though I've tested the new behaviour on a local project - do you think this is a problem?

ghostdogpr
ghostdogpr approved these changes Jul 7, 2021
View reviewed changes
Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for the contribution!

@ghostdogpr ghostdogpr merged commit 7f2d8ce into ghostdogpr:master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghostdogpr ghostdogpr ghostdogpr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LaurenceWarne @ghostdogpr